adapter: decompose ZSet storage into per-member wide-column keys#483
adapter: decompose ZSet storage into per-member wide-column keys#483
Conversation
bootjp
force-pushed
the
feat/reduce-lock
branch
from
9c1b322 to
37d6099
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions Redis Sorted Set (ZSet) storage from a single legacy blob to a wide-column format, where members and their scores are stored in individual keys alongside a score-based index. The changes include new helper functions for managing these keys, logic to migrate existing data upon access, and updates to ZSet commands to support incremental, diff-based updates. Feedback highlights an issue where empty ZSets might incorrectly persist their metadata key instead of being deleted, and suggests consistent use of the store.PrefixEnd utility to avoid naming inconsistencies.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates Redis Sorted Set (ZSet) storage from a legacy blob format to a wide-column layout using per-member keys and a score index. The changes include new key construction logic, sortable float encoding, and updated command implementations to support incremental updates and on-the-fly migration. Feedback focuses on optimizing storage operations by utilizing the new score index for ordered access and using prefix deletions to avoid inefficient full-set scans and potential memory exhaustion.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a wide-column storage format for Redis Sorted Sets, replacing the legacy blob-based storage with a more granular structure of metadata, member, and score-index keys. The changes include new key encoding logic, updated transaction state management for diff-based updates, and modified Redis command handlers to support the new format and facilitate on-the-fly migration. The review feedback highlights several areas for improvement, including optimizing bulk deletions via prefix-based operations, preventing potential OOM issues by bounding member scans, and refining migration logic to handle empty sets more efficiently.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a wide-column storage format for Redis Sorted Sets, transitioning from a single-blob model to a structure using metadata, member keys, and a score index. The changes include migration support, updated transaction state, and revised command implementations. Feedback focuses on critical atomicity risks in prefix deletions, potential memory exhaustion during unbounded scans, and performance bottlenecks in the O(N) diffing and range-scanning logic.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a wide-column storage format for Redis Sorted Sets (ZSets), utilizing metadata, member, and score-index keys to replace the legacy single-blob format. The implementation includes migration logic and updates to ZSet commands to support the new structure. Feedback identifies a bug where new prefixes were not registered for KEYS and SCAN commands. Additionally, reviewers raised concerns about potential out-of-memory (OOM) vulnerabilities and Raft limit issues due to operations that load entire sets into memory or perform unbounded scans during deletions and updates.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a wide-column storage format for Redis Sorted Sets (ZSets), moving away from the legacy single-blob approach to a per-member key structure with a dedicated score index. The changes include new key construction logic in store/zset_helpers.go, migration logic to handle legacy data, and updates to various Redis commands like ZADD, ZREM, and ZRANGE to support the new format. Feedback focuses on ensuring consistent score encoding for zero values and addressing potential performance and memory issues when deleting large sets by avoiding unbounded scans and excessive mutation batches.
| func EncodeSortableFloat64(dst []byte, f float64) { | ||
| if len(dst) < zsetScoreBinarySize { | ||
| return | ||
| } | ||
| bits := math.Float64bits(f) | ||
| if bits&(1<<float64SignBitPos) != 0 { // negative | ||
| bits = ^bits | ||
| } else { | ||
| bits ^= 1 << float64SignBitPos | ||
| } | ||
| binary.BigEndian.PutUint64(dst, bits) | ||
| } |
There was a problem hiding this comment.
The current float64 encoding does not normalize -0.0 to +0.0. In IEEE 754, these have different bit patterns (0x8000000000000000 vs 0x0000000000000000), which results in different sortable byte sequences (0x7FFFFFFFFFFFFFFF vs 0x8000000000000000). Since Redis treats -0.0 and +0.0 as equal scores, they should have the same representation in the score index to ensure consistent range scans and ordering.
func EncodeSortableFloat64(dst []byte, f float64) {
if len(dst) < zsetScoreBinarySize {
return
}
if f == 0 {
f = 0.0 // normalize -0.0 to +0.0
}
bits := math.Float64bits(f)
if bits&(1<<float64SignBitPos) != 0 { // negative
bits = ^bits
} else {
bits ^= 1 << float64SignBitPos
}
binary.BigEndian.PutUint64(dst, bits)
}
adapter/redis_compat_helpers.go
Outdated
| store.ZSetMemberScanPrefix(key), | ||
| store.ZSetScoreScanPrefix(key), | ||
| } { | ||
| kvs, err := r.store.ScanAt(ctx, prefix, store.PrefixEnd(prefix), math.MaxInt, readTS) |
There was a problem hiding this comment.
Using math.MaxInt as a limit for ScanAt when collecting keys for deletion can lead to unbounded memory growth and potential OOM issues if a ZSet contains a very large number of members. This violates the general rule to apply fixed bounds to collections that can grow from external requests. Consider using kv.DelPrefix for the member and score prefixes if the storage engine supports it, or implementing a chunked deletion approach to keep memory usage under control.
References
- To prevent unbounded memory growth and potential OOM issues, apply a fixed bound to collections that can grow from external requests.
| func (r *RedisServer) deleteZSetWideColumnElems(ctx context.Context, key []byte, readTS uint64) ([]*kv.Elem[kv.OP], error) { | ||
| _, metaExists, err := r.loadZSetMetaAt(ctx, key, readTS) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if !metaExists { | ||
| return nil, nil | ||
| } | ||
|
|
||
| elems := []*kv.Elem[kv.OP]{ | ||
| {Op: kv.Del, Key: store.ZSetMetaKey(key)}, | ||
| } | ||
|
|
||
| // Scan member keys and score-index keys for individual deletion. | ||
| for _, prefix := range [][]byte{ | ||
| store.ZSetMemberScanPrefix(key), | ||
| store.ZSetScoreScanPrefix(key), | ||
| } { | ||
| kvs, err := r.store.ScanAt(ctx, prefix, store.PrefixEnd(prefix), math.MaxInt, readTS) | ||
| if err != nil { | ||
| return nil, errors.WithStack(err) | ||
| } | ||
| for _, kvp := range kvs { | ||
| elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Del, Key: bytes.Clone(kvp.Key)}) | ||
| } | ||
| } | ||
|
|
||
| return elems, nil |
There was a problem hiding this comment.
The deleteZSetWideColumnElems function generates individual Del operations for every member and score index key. For large ZSets, this results in a massive number of operations being batched into a single Raft request, which can bloat the Raft log, increase apply latency, and potentially exceed message size limits. If kv.DelPrefix cannot be mixed with other mutations, consider if the DEL command logic can be refactored to use prefix deletion for ZSet internal data namespaces.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates Redis Sorted Set (ZSet) storage from a legacy single-blob format to a wide-column structure, utilizing separate keys for metadata, members, and score-based indexing. The changes include new helper functions for incremental updates, member scanning, and migration logic to ensure compatibility with existing data. Review feedback highlights a potential memory risk when scanning large sets with unbounded limits and suggests an optimization to avoid redundant metadata writes when the set's length remains unchanged.
adapter/redis_compat_helpers.go
Outdated
| kvs, err := r.store.ScanAt(ctx, prefix, store.PrefixEnd(prefix), math.MaxInt, readTS) | ||
| if err != nil { | ||
| return nil, errors.WithStack(err) | ||
| } | ||
| for _, kvp := range kvs { | ||
| elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Del, Key: bytes.Clone(kvp.Key)}) | ||
| } | ||
| } |
There was a problem hiding this comment.
The use of math.MaxInt for the scan limit can lead to unbounded memory growth and potential OOM issues when deleting a very large ZSet. To prevent this, apply a fixed bound to collections that can grow from external requests. Consider implementing a batched scan-and-delete approach to process keys in smaller chunks, which would mitigate the memory pressure.
References
- To prevent unbounded memory growth and potential OOM issues, apply a fixed bound to collections that can grow from external requests, such as pending configuration changes. Reject new requests when the bound is reached.
adapter/redis_compat_helpers.go
Outdated
| } else if len(elems) > 0 { | ||
| metaBytes, err := store.MarshalZSetMeta(store.ZSetMeta{Len: int64(len(newMembers))}) | ||
| if err != nil { | ||
| return nil, errors.WithStack(err) | ||
| } | ||
| elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Put, Key: store.ZSetMetaKey(key), Value: metaBytes}) | ||
| } |
There was a problem hiding this comment.
The meta key is currently rewritten on every diff that modifies any element, even if the total number of members in the ZSet does not change (e.g., when only a score is updated). Since the meta key only stores the length, this write is redundant. Consider updating the logic to only write the meta key when the length actually changes.
| } else if len(elems) > 0 { | |
| metaBytes, err := store.MarshalZSetMeta(store.ZSetMeta{Len: int64(len(newMembers))}) | |
| if err != nil { | |
| return nil, errors.WithStack(err) | |
| } | |
| elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Put, Key: store.ZSetMetaKey(key), Value: metaBytes}) | |
| } | |
| } else if len(origMembers) != len(newMembers) { | |
| metaBytes, err := store.MarshalZSetMeta(store.ZSetMeta{Len: int64(len(newMembers))}) | |
| if err != nil { | |
| return nil, errors.WithStack(err) | |
| } | |
| elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Put, Key: store.ZSetMetaKey(key), Value: metaBytes}) | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a wide-column storage format for Redis Sorted Sets, transitioning from a single legacy blob to a more granular structure using metadata, member, and score-index keys. The changes include new key construction utilities, updated transaction states to support diff-based commits, and refactored Redis commands (e.g., ZADD, ZRANGE, BZPOPMIN) that handle both storage formats. Feedback was provided regarding the zrangeRead implementation, where a slice returned from the storage layer is reversed in-place; it is recommended to copy this slice first to prevent unintended side effects on shared data during the legacy migration phase.
| entries, err := r.loadZSetEntriesSorted(context.Background(), key, readTS) | ||
| if err != nil { | ||
| conn.WriteError(err.Error()) | ||
| return | ||
| } |
There was a problem hiding this comment.
The reverseZSetEntries function modifies its slice argument in-place. The entries slice returned from loadZSetEntriesSorted might be backed by a shared array, especially when falling back to the legacy blob format which involves unmarshaling. Modifying it directly could lead to unexpected side effects. It's safer to create a copy before performing the in-place reversal, as was done in the previous implementation.
entries, err := r.loadZSetEntriesSorted(context.Background(), key, readTS)
if err != nil {
conn.WriteError(err.Error())
return
}
// Make a copy before potential in-place reversal.
entries = append([]redisZSetEntry(nil), entries...)…review - Address PR #485 gemini review (4094524421): - Add stale claim key cleanup in PUSH write path - Enforce contiguous-only Head/Tail advancement in compaction - Add explicit RPOP claim flow (reverse scan from Tail-1) - Add Tail-side GC for RPOP claim keys - Extend delta metadata pattern to Hash, Set, and ZSet: - Hash: wide-column decomposition + delta - Set: wide-column decomposition + delta - ZSet: delta on existing wide-column (PR #483) - Unified compactor, shared delta limits, transition plan per type - Rename list_metadata_delta_design.md to collection_metadata_delta_design.md
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a wide-column storage format for Redis Sorted Sets, replacing the legacy single-blob storage with granular keys for metadata, members, and a score-based index. The implementation includes migration logic and support for standard ZSet commands. Review feedback highlights significant scalability risks, noting that several operations still load entire sets into memory or generate excessively large transaction batches, which could lead to OOM errors or cluster instability with large datasets.
| func (r *RedisServer) loadZSetMembersMap(ctx context.Context, key []byte, readTS uint64) (zsetLoadResult, error) { | ||
| // Check wide-column meta first. | ||
| meta, metaExists, err := r.loadZSetMetaAt(ctx, key, readTS) | ||
| if err != nil { | ||
| return zsetLoadResult{}, err | ||
| } | ||
| if metaExists { | ||
| entries, err := r.scanZSetAllMembers(ctx, key, meta.Len, readTS) | ||
| if err != nil { | ||
| return zsetLoadResult{}, err | ||
| } | ||
| return zsetLoadResult{members: zsetEntriesToMap(entries), exists: true}, nil | ||
| } | ||
| // Fall back to legacy blob format. | ||
| value, exists, err := r.loadZSetAt(ctx, key, readTS) | ||
| if err != nil { | ||
| return zsetLoadResult{}, err | ||
| } | ||
| if !exists { | ||
| return zsetLoadResult{members: map[string]float64{}}, nil | ||
| } | ||
| return zsetLoadResult{members: zsetEntriesToMap(value.Entries), exists: true, fromLegacy: true}, nil | ||
| } |
There was a problem hiding this comment.
The loadZSetMembersMap function loads the entire sorted set into an in-memory map. For large sets, this can lead to excessive memory consumption and potential OOM (Out of Memory) errors on the adapter node. This monolithic access pattern defeats the scalability benefits of the new wide-column storage format. Consider refactoring commands like ZADD, ZREM, and ZSCORE to use point lookups or range scans instead of loading the full set.
References
- To prevent unbounded memory growth and potential OOM issues, apply a fixed bound to collections that can grow from external requests, such as pending configuration changes. Reject new requests when the bound is reached.
| } | ||
|
|
||
| value, _, err := c.server.loadZSetAt(context.Background(), key, c.startTS) | ||
| load, err := c.server.loadZSetMembersMap(context.Background(), key, c.startTS) |
There was a problem hiding this comment.
Similar to the adapter commands, Lua scripts also load the entire ZSet into memory via loadZSetMembersMap. This poses a significant OOM risk for large sets processed within scripts. Consider providing Lua APIs that can perform partial updates or range scans on ZSets.
References
- To prevent unbounded memory growth and potential OOM issues, apply a fixed bound to collections that can grow from external requests, such as pending configuration changes. Reject new requests when the bound is reached.
| func (r *RedisServer) deleteZSetWideColumnElems(ctx context.Context, key []byte, readTS uint64) ([]*kv.Elem[kv.OP], error) { | ||
| meta, metaExists, err := r.loadZSetMetaAt(ctx, key, readTS) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if !metaExists { | ||
| return nil, nil | ||
| } | ||
|
|
||
| elems := []*kv.Elem[kv.OP]{ | ||
| {Op: kv.Del, Key: store.ZSetMetaKey(key)}, | ||
| } | ||
|
|
||
| // Use meta.Len to bound the scan. Each member has one member key and one | ||
| // score-index key, so we expect exactly meta.Len entries per prefix. | ||
| scanLimit := int(meta.Len) | ||
|
|
||
| // Scan member keys and score-index keys for individual deletion. | ||
| for _, prefix := range [][]byte{ | ||
| store.ZSetMemberScanPrefix(key), | ||
| store.ZSetScoreScanPrefix(key), | ||
| } { | ||
| kvs, err := r.store.ScanAt(ctx, prefix, store.PrefixEnd(prefix), scanLimit, readTS) | ||
| if err != nil { | ||
| return nil, errors.WithStack(err) | ||
| } | ||
| for _, kvp := range kvs { | ||
| elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Del, Key: bytes.Clone(kvp.Key)}) | ||
| } | ||
| } | ||
|
|
||
| return elems, nil | ||
| } |
There was a problem hiding this comment.
deleteZSetWideColumnElems performs a full scan of all member and score index keys and generates individual kv.Del operations for each. For large ZSets, this creates a massive transaction that can exceed Raft message size limits and cause cluster instability. Additionally, the scan itself can lead to OOM if the number of keys is very large. Consider implementing a more scalable deletion strategy, such as asynchronous background cleanup or using a single prefix deletion if the system constraints allow.
References
- To prevent unbounded memory growth and potential OOM issues, apply a fixed bound to collections that can grow from external requests, such as pending configuration changes. Reject new requests when the bound is reached.
| return 0, wrongTypeError() | ||
| } | ||
| value, _, err := r.loadZSetAt(context.Background(), key, readTS) | ||
| load, err := r.loadZSetMembersMap(context.Background(), key, readTS) |
There was a problem hiding this comment.
zaddTxn (and similarly zincrby at line 1399) loads the entire ZSet map to perform updates. This is inefficient for large sets. Point lookups using the new wide-column keys should be used to check for member existence and retrieve old scores, allowing for O(1) updates regardless of the set size.
References
- To prevent unbounded memory growth and potential OOM issues, apply a fixed bound to collections that can grow from external requests, such as pending configuration changes. Reject new requests when the bound is reached.
[Test Coverage] レビュー結果PR #483: adapter: decompose ZSet storage into per-member wide-column keys変更ファイル: 問題点1. 新規ファイル
|
| 新機能 | テスト有無 |
|---|---|
store/zset_helpers.go 全関数 |
なし |
EncodeSortableFloat64 のスコートソート順 |
なし |
| ZSet ワイドカラム差分コミット | なし |
fromLegacy マイグレーションパス |
部分的（既存テスト更新のみ） |
スコアインデックスの正確性は ZRANGE/ZRANGEBYSCORE の正確さに直結するため、特に EncodeSortableFloat64 の境界値テストを強く推奨します。
[Performance] レビュー結果PR #483 の性能観点のレビューです。ZSet をワイドカラム形式に分解する設計変更自体は I/O 削減の方向性として正しいですが、以下の問題点があります。 🔴 重大: ZADD/ZINCRBY/ZREM がワイドカラムパスでも O(N) の全メンバーロード該当箇所: // zaddTxn — 1メンバー追加でも全件スキャン
load, err := r.loadZSetMembersMap(context.Background(), key, readTS)
// → loadZSetMetaAt(1 I/O) + scanZSetAllMembers(N I/O) + zsetEntriesToMap(O(N) alloc)ワイドカラム化の主なメリットは「単一メンバー操作を O(1) I/O にする」ことですが、現実装では 提案: ZADD/ZINCRBY は「古いスコアキーを取得する → member key のみ点読み (O(1) I/O)」で実現できます。 🟠 重要: 書き込みのたびに O(N) のマップコピーが 2 回発生該当箇所: // loadZSetState — origMembers スナップショット
origMembers := make(map[string]float64, len(load.members))
for mk, mv := range load.members {
origMembers[mk] = mv
}
// 呼び出し元 (例: zaddTxn) — さらにもう 1 コピー
members := make(map[string]float64, len(load.members))
for k, v := range load.members {
members[k] = v
}単一メンバー操作でも常に「全メンバーマップ × 2」のメモリを確保しています。N=10,000 なら 16 バイト × 10,000 × 2 = 320KB のアロケーションが毎回発生します。 🟠 重要:
|
| 優先度 | 問題 | 影響 |
|---|---|---|
| 🔴 重大 | ZADD 等が wide-column でも全件 O(N) I/O ロード | 大規模 ZSet で性能劣化 |
| 🟠 重要 | 書き込み毎に O(N) map × 2 コピー | メモリ圧力・GC 負荷増 |
| 🟠 重要 | delete で member/score を別々にスキャン | I/O 2× |
| 🟡 軽微 | buildZSetDiffElems cap=0 |
realloc によるパフォーマンスロス |
| 🟡 軽微 | キー構築で 3 alloc/キー | ホットパスでの GC 圧 |
[Consistency] レビュー結果PR #483: adapter: decompose ZSet storage into per-member wide-column keys 問題: スコアのみ変更時にメタキーが更新されず、競合検出が機能しない（線形化可能性違反）
} else if len(origMembers) != len(newMembers) {
// Only update meta if cardinality changed
elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Put, Key: store.ZSetMetaKey(key), Value: metaBytes})
}メンバー数が変わらない場合（スコア更新: ZADD/ZINCRBY で既存メンバーのスコアだけ変わるケース）、メタキー ( 一方、 t.trackReadKey(store.ZSetMetaKey(key))個々のメンバーキー ( 問題のシナリオ（Lost Update）:
結果: 修正案: メンバー数が変わらない場合でも、スコア変更があれば常にメタキーを更新する（値を同一のままでも書き込む）か、個々のメンバーキー・スコアインデックスキーも読み取りセットに追加する。 例えばメタキーを常に書き込む場合: // len(origMembers) != len(newMembers) の条件を削除し、len(newMembers) > 0 なら常に更新
metaBytes, err := store.MarshalZSetMeta(store.ZSetMeta{Len: int64(len(newMembers))})
if err != nil {
return nil, errors.WithStack(err)
}
elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Put, Key: store.ZSetMetaKey(key), Value: metaBytes})この変更により、ZINCRBY/ZADD（スコア更新）でもメタキーへの書き込みが競合検出のアンカーとなり、線形化可能性が保たれます。 |
[Performance] レビュー結果P1: キー構築関数での多重アロケーション（ホットパス・ループ内）store/zset_helpers.go:43–103
// 現状: ZSetMemberKey の例
func ZSetMemberKey(userKey, member []byte) []byte {
prefix := []byte(ZSetMemberPrefix) // alloc 1
comp := zsetUserKeyComponent(userKey) // alloc 2 (内部make)
buf := make(...) // alloc 3
...
}改善案： func ZSetMemberKey(userKey, member []byte) []byte {
prefixLen := len(ZSetMemberPrefix)
buf := make([]byte, 0, prefixLen+zsetUserKeyLenSize+len(userKey)+len(member))
buf = append(buf, ZSetMemberPrefix...)
binary.BigEndian.AppendUint32(buf[len(buf):len(buf)+zsetUserKeyLenSize], uint32(len(userKey)))
buf = buf[:len(buf)+zsetUserKeyLenSize]
buf = append(buf, userKey...)
buf = append(buf, member...)
return buf
}P2:
|
[Consistency] レビュー結果対象: adapter: decompose ZSet storage into per-member wide-column keys指摘事項: 2件 [重大]
|
[Data Loss] レビュー結果1 件の重大なリスクと 1 件の軽微なリスクを確認しました。 [高] FLUSHALL/FLUSHDB が
|
| プレフィックス | 用途 |
|---|---|
| `!zs | meta |
| `!zs | mem |
| `!zs | scr |
一方、PR #486 で変更された FLUSHALL/FLUSHDB は !redis| と !lst| のみを削除対象とし、!zs| プレフィックスは対象外です。
// PR #486 の flushDatabase（変更後）
{Op: kv.DelPrefix, Key: []byte("!redis|")},
{Op: kv.DelPrefix, Key: []byte("!lst|")},
// !zs|meta|, !zs|mem|, !zs|scr| は削除されない両 PR がマージされると、FLUSHALL/FLUSHDB を実行しても ZSet データが残存します。ユーザーは Redis の FLUSHALL がデータを完全に消去すると期待するため、これは深刻な動作不一致です。
対処案: PR #486 の flushDatabase に {Op: kv.DelPrefix, Key: []byte("!zs|")} を追加する。
[低] deleteZSetWideColumnElems のスキャン上限が meta.Len に依存
該当箇所: adapter/redis_compat_helpers.go の deleteZSetWideColumnElems
scanLimit := int(meta.Len)
for _, prefix := range [][]byte{
store.ZSetMemberScanPrefix(key),
store.ZSetScoreScanPrefix(key),
} {
kvs, err := r.store.ScanAt(ctx, prefix, store.PrefixEnd(prefix), scanLimit, readTS)
// ← meta.Len より多くキーが存在した場合、末尾のキーが削除されない通常は各書き込みトランザクション内でメンバー数と meta.Len が同期されるため問題になりませんが、万一 meta.Len が実キー数を下回る状態（例: 将来のバグ等による不整合）になると、削除しきれない孤立キーがストレージに残ります。ユーザーからは ZSet が削除されたように見えますが、内部には古いキーが残存します（ストレージリーク）。
[Concurrency] レビュー結果PR: adapter: decompose ZSet storage into per-member wide-column keys 問題あり（中程度）1. ミュータブルな map を別名で渡している（latent aliasing bug）該当箇所: コード例（bzPopMinLegacy）: delete(load.members, popped.Member) // load.members を直接変更
if err := r.persistZSetMembersTxn(ctx, key, readTS, load, load.members); err != nil {問題: 現在は安全な理由: レガシーパスでは 潜在的リスク: 将来のリファクタリングや、データ不整合によって 同様のパターン: 推奨修正: // bzPopMinLegacy: newMembers は load.members のコピーを削除後に作成する
newMembers := make(map[string]float64, len(load.members))
for k, v := range load.members {
newMembers[k] = v
}
delete(newMembers, popped.Member)
if err := r.persistZSetMembersTxn(ctx, key, readTS, load, newMembers); err != nil {2. context.Background() の使用（既存の問題だが範囲拡大）該当箇所: 外部から渡された 3. meta.Len を scan limit に使用（軽微）
|
[Performance] レビュー結果PR #483: adapter: decompose ZSet storage into per-member wide-column keys[P1] 読み取り専用アクセスでの不要な O(N) マップコピー場所: origMembers := make(map[string]float64, len(load.members))
for mk, mv := range load.members {
origMembers[mk] = mv
}
改善案: [P2] zaddTxn / zincrby での二重マップアロケーション場所: load, err := r.loadZSetMembersMap(...) // map を新規確保
members := make(map[string]float64, len(load.members)) // さらにコピー
for k, v := range load.members {
members[k] = v
}
[P3] deleteZSetWideColumnElems での二回の ScanAt場所: for _, prefix := range [][]byte{
store.ZSetMemberScanPrefix(key),
store.ZSetScoreScanPrefix(key),
} {
kvs, err := r.store.ScanAt(ctx, prefix, store.PrefixEnd(prefix), scanLimit, readTS)
...
}各プレフィックスが連続した範囲を占めていれば1回の広めのスキャンで代替できる可能性がある。構造上2スキャン不可避であれば問題なし。ただし |
[Consistency] レビュー結果問題なし。 確認した主要ポイントWide-column ZSet の meta/member/score キー整合性:
Conflict detection の正確性（
Legacy blob → wide-column マイグレーションのアトミック性:
TTL と ZSet データ本体の整合性:
|
[Data Loss] レビュー結果対象PR: #483 adapter: decompose ZSet storage into per-member wide-column keys 問題1 (MEDIUM):
|
[Data Loss] レビュー結果対象PR: #483 adapter: decompose ZSet storage into per-member wide-column keys結論: 潜在的なデータ可視性リスクあり（要確認）懸念事項
|
[Performance] レビュー結果PR #483: adapter: decompose ZSet storage into per-member wide-column keys1. [高影響] 削除時の無制限スキャン (,
|
[Data Loss] レビュー結果対象PR: adapter: decompose ZSet storage into per-member wide-column keys (#483)問題 1 (重大):
|
[Consistency] レビュー結果✅ 正しい実装meta/member/score キーの原子的コミット
|
[Data Loss] レビュー結果対象PR: #483 adapter: decompose ZSet storage into per-member wide-column keys 要確認: 1件の潜在的データ損失パス + 1件の事前存在バグ[要確認]
|
[Concurrency] レビュー結果PR #483 —
|
[Performance] レビュー結果
|
[Test Coverage] レビュー結果PR #483: adapter: decompose ZSet storage into per-member wide-column keysカバーされているテスト ✅
テスト不足 ❌1.
|
[Concurrency] レビュー結果PR #483: adapter: decompose ZSet storage into per-member wide-column keys問題なし。 以下の点を確認しました：
|
[Performance] レビュー結果問題あり1. 単一メンバー操作でも全メンバーをロードする（重大）場所:
例: 10,000 件のメンバーを持つ ZSet への 改善案: diff-based commit を維持しつつ、差分対象メンバーだけをロードする専用パス（ 2. ZSet ロード時に origMembers の二重マップ確保（中程度）場所: ZSet をロードするたびに:
read-only な ZSet アクセス（ 改善案: 3.
|
[Consistency] レビュー結果問題あり: TTL 整合性バグ（Critical）1. [Critical] 期限切れキーへの ZADD が TTL キーをクリアしない問題箇所:
case len(st.origMembers) == 0:
// Brand new ZSet: full write
return buildZSetWriteElems(keyBytes, st.members) // TTL キーに触れない
結果として：
対比: 同じファイル内の case len(st.members) == 0 && st.fromLegacy:
return []*kv.Elem[kv.OP]{
{Op: kv.Del, Key: redisZSetKey(keyBytes)},
{Op: kv.Del, Key: redisTTLKey(keyBytes)}, // ← こちらは正しく削除
}, nil修正案: 2. キーエンコーディングの一貫性
3. diff ベースコミットのアトミック性
4. メタカウントとメンバー数の整合性
5. レガシー移行のアトミック性
まとめ
|
[Consistency] レビュー結果対象PR: #483 (adapter: decompose ZSet storage into per-member wide-column keys) 🔴 [重要]
|
| # | 分類 | 内容 |
|---|---|---|
| 1 | 🔴 重要 | wide-columnケースでのTTL削除非対称性 — 孤立TTLキーのリスク |
| 2 | buildZSetDiffElems 削除が origMembers のみ対象（孤立キー対策として deleteZSetWideColumnElems の使用を検討） |
[Performance] レビュー結果[高]
|
[Performance] レビュー結果[高]
|
[Concurrency] レビュー結果PR 483: ZSetストレージ分解 (adapter: decompose ZSet storage into per-member wide-column keys)1.
|
[Performance] レビュー結果PR #483: adapter: decompose ZSet storage into per-member wide-column keys1.
|
[Data Loss] レビュー結果PR #483: adapter: decompose ZSet storage into per-member wide-column keys🔴 [重大] 全削除時に並行書き込みメンバーが orphaned key として残留 ─ データロスト場所: ZSet の全メンバーを削除する際（ case len(st.members) == 0:
// Delete meta + all member/score keys via prefix deletion.
return buildZSetDiffElems(keyBytes, st.origMembers, st.members)
具体的なデータロストシナリオ（シングルシャード構成）:
なぜ SSI で防げるはずなのに防げないか: PR #483 では 推奨対応:
🟡 [要注意]
|
| 重大度 | 問題 |
|---|---|
| 🔴 重大 | 全削除時に並行書き込みメンバーが orphaned key 化 → データロスト（PR #499 の SSI ギャップと組み合わせで確実に発生） |
| 🟡 要注意 | meta.Len 不整合時の早期メタキー削除 → 残存メンバーが不可視 |
| 🟡 要注意 | scanZSetAllMembers の meta.Len 依存 → 超過メンバーの読み取り漏れ |
PR #499 (
validateReadSet実装) と本PR はセットでマージされることを強く推奨します。本PR 単独では、シングルシャード構成で全削除 + 並行追加のシナリオにてデータロストが発生し得ます。
[Consistency] レビュー結果PR #483: adapter: decompose ZSet storage into per-member wide-column keys[重大] score-only 更新時に
|
[Performance] レビュー結果問題1:
|
[Test Coverage] レビュー結果PR #483: adapter: decompose ZSet storage into per-member wide-column keys 十分にカバーされている箇所
不足しているテスト1.
|
| 分岐 | 条件 | テスト |
|---|---|---|
| レガシーの全削除 | len(members)==0 && fromLegacy |
❌ |
| wide-columnの全削除 | len(members)==0 |
buildZSetDiffElems経由で間接的のみ |
| レガシー→wide-columnマイグレーション | fromLegacy |
redis_storage_migration_testで間接的のみ |
| 新規ZSet作成 | len(origMembers)==0 |
❌ |
提案するテストケース:
func TestBuildZSetStateElems_LegacyEmpty(t *testing.T) { ... }
func TestBuildZSetStateElems_NewZSet(t *testing.T) { ... }
func TestBuildZSetStateElems_LegacyMigration(t *testing.T) { ... }2. zremrangebyrankInner のテストなし (高優先度)
wide-column path と legacy blob fallback の2パスを持つ新関数だが、どちらもユニットテスト・統合テストが存在しない。
提案するテストケース:
- wide-column:
ZREMRANGEBYRANK key 0 -1（全削除） - wide-column: 範囲が空（s > e）の場合
removed=0を返す - legacy blob fallback: 旧フォーマットデータへの
ZREMRANGEBYRANK
3. bzPopMinWideColumn / bzPopMinLegacy のテストなし
tryBZPopMinInner がwide-column/legacyの両パスに分岐する新実装だが、テストがない。
提案するテストケース:
- wide-column: 最低スコアのメンバーがpopされ、meta.Lenが更新される
- wide-column: ZSetが空の場合
nilを返す - legacy: レガシーデータからのBZPOPMIN
4. wide-columnフォーマットへの純粋な書き込み・読み込みの統合テストなし
redis_storage_migration_test.go はレガシーからの移行テストだが、最初からwide-columnに書き込んで読み込むシナリオのテストがない。
提案するテストケース:
// ZADD → ZRANGE → ZREM → ZCARDを、レガシーデータなしの
// 新規wide-columnデータでエンドツーエンドに確認5. loadZSetMembersMap wide-columnパスの単体テストなし
loadZSetMembersMap はwide-column first → legacy fallbackの順で検索するが、wide-columnが存在するケースの単体テストがない。
6. rawKeyTypeAt でのZSetキー種別検出テストなし
rawKeyTypeAt が ZSetMetaKey と レガシー redisZSetKey の両方をチェックするように変更されたが、各フォーマットでの TYPE コマンド動作テストがない。
7. redisVisibleUserKey のZSetInternalKeyテストなし
IsZSetInternalKey ブランチが追加されたが、localKeysPattern / KEYS コマンドでwide-columnキーからユーザーキーが正しく見えることのテストがない。
8. 大量メンバーのZSetに対するテストなし
scanZSetAllMembers が meta.Len を上限として使用するが、meta.Len が実際のエントリ数より小さい場合（不整合時）の動作テストが不足。
まとめ
| 項目 | 評価 |
|---|---|
buildZSetDiffElems ユニットテスト |
✅ 十分 |
| ZSetキーエンコーディング | ✅ 十分 |
| レガシー→wide-column マイグレーション | ✅ 基本カバー |
buildZSetStateElems 各分岐 |
❌ 未テスト |
zremrangebyrankInner |
❌ テストなし |
bzPopMinWideColumn / bzPopMinLegacy |
❌ テストなし |
| wide-column純粋書き込み統合テスト | ❌ テストなし |
loadZSetMembersMap wide-columnパス |
❌ 単体テストなし |
[Consistency] レビュー結果PR #483: adapter: decompose ZSet storage into per-member wide-column keys🔴 問題1:
|
[Performance] レビュー結果[中]
|
[Concurrency] レビュー結果対象PR: #483 adapter: decompose ZSet storage into per-member wide-column keys Info: diff-based コミットのアトミック性（問題なし）場所: スナップショット取得（ Info: レガシー→ワイドカラム移行のアトミック性（問題なし）場所: ワイドカラム形式への書き込みとレガシーblobの削除が単一の Info: プレフィックススキャンのキー衝突（問題なし）場所: ZSetメタキーに4バイト長プレフィックス（ Info: 型読み取りキーの追跡（問題なし）場所:
総評Race condition、デッドロック、goroutineリーク、ブロッキングチャネルはいずれも検出されませんでした。スナップショット・diff・アトミックdispatchのパターンが一貫して適用されており、並行性の観点では問題なしと判断します。 |
[Test Coverage] レビュー結果PR #483: adapter: decompose ZSet storage into per-member wide-column keys十分にカバーされている箇所
テスト不足の箇所1. 5つの分岐があるがテストがゼロ。コミット時の中核ロジック。提案するテストケース: // 分岐1: len(members)==0 && fromLegacy → legacyキーとTTLキーをDel
// 分岐2: len(members)==0 (wide-column) → meta+全member/scoreキーをDel
// 分岐3: fromLegacy=true → wide-columnへの完全書き込み + legacyキーDel（マイグレーション）
// 分岐4: len(origMembers)==0 → 新規ZSet完全書き込み
// 分岐5: 通常のdiff更新2. テストなし。 // fromLegacy=true: wide-column書き込み + legacyキー削除の両方が発行されること
// fromLegacy=false: buildZSetDiffElemsが呼ばれること
// newMembers空: deleteLogicalKeyElemsにフォールバックすること3. wide-columnとlegacyの2パスがあるがテストなし。 4. リファクタリングで2パス（wide-column / legacy）に分かれたがテストなし。提案するテストケース: // wide-columnパス: metaExistsで正しいrank範囲のエントリが削除されること
// legacyパス: legacyフォールバックで正しく動作すること
// 境界値: start/stopが実際のエントリ数を超える場合（e < s → removed=0）5. テストなし。 // wide-column空ZSet: result=nil が返ること
// wide-column通常: 最小スコアのエントリがpopされ、wide-columnキーが削除されること
// legacyパス: legacy形式で正しく動作すること6. 新規関数だがテストなし。 7. ZSet内部キーに対するTTL有効期限チェックの新コードパスのテストなし。 8. KEYSコマンドが wide-column ZSetキーを返すかのテストなし。 // wide-column形式でZADDしたキーがKEYSコマンドで返ること9. PR #499のSSI機能と連携する重要な変更だが、ZSetメタキーがreadKeyとして正しく追跡されるかのテストなし。 10. 並行アクセステスト wide-column ZSet操作（ZADD、ZREM、ZREMRANGEBYRANK）に対して並行テストがない。従来はlocked blobs単一キーだったが、wide-columnは複数キーへの分散書き込みになるため並行安全性の検証が特に重要。 まとめストアヘルパー（key encoding/decoding）とdiff計算はよくテストされていますが、実際のZSet操作（write/read/delete）のend-to-endテストと** |
[Performance] レビュー結果PR #483: adapter: decompose ZSet storage into per-member wide-column keys1.
|
[Test Coverage] レビュー結果対象PR: #483 adapter: decompose ZSet storage into per-member wide-column keys テストが追加されているロジック
テストが不足している箇所1.
|
[Consistency] レビュー結果PR #483: adapter: decompose ZSet storage into per-member wide-column keys問題1: レガシーZSetの読み取りキー追跡の欠落 [重要]該当箇所:
t.trackReadKey(store.ZSetMetaKey(key)) // メタキーのみ追跡しかし
レガシー形式のZSetに対して実際に読み取られるのは PR #499のSSIと組み合わせたとき: 修正案: 問題2:
|
[Data Loss] レビュー結果対象PR: #483 adapter: decompose ZSet storage into per-member wide-column keys 結論: データロストリスクなし（ただし2点の注意事項あり）確認内容1. レガシー blob → ワイドカラム移行のアトミック性
2. buildZSetDiffElems のスコアインデックス整合性スコア変更時に旧スコアインデックスキーを削除し新スコアインデックスキーを追加する操作は、同一 dispatch でアトミック。孤児スコアインデックスキーは生まれない。✅ 3. deleteLogicalKeyElems でのワイドカラム ZSet 削除
4. bzPopMinWideColumn の「先取り set」問題
5. buildZSetStateElems - legacy empty パス
注意事項1: meta.Len に依存したスキャン上限（理論的リスク）場所: func (r *RedisServer) scanZSetAllMembers(..., memberCount int64, ...) {
return r.scanZSetScoreEntries(ctx, key, int(memberCount), readTS)
}meta.Len が実エントリ数より小さい場合、 注意事項2: deleteZSetWideColumnElems の TOCTOU（実害なし）場所:
データロスト判定: 問題なし |
[Data Loss] レビュー結果対象PR: #483 (adapter: decompose ZSet storage into per-member wide-column keys) 問題1 【中】トランザクション内でZSet全削除時にTTLキーが残存 → 再作成後のデータ消失リスク該当箇所: case len(st.members) == 0 && st.fromLegacy:
// fromLegacy=true のとき: legacyブロブ + TTLキー を削除 ✓
return []*kv.Elem[kv.OP]{
{Op: kv.Del, Key: redisZSetKey(keyBytes)},
{Op: kv.Del, Key: redisTTLKey(keyBytes)}, // ← TTL削除あり
}, nil
case len(st.members) == 0:
// wide-column のとき: meta + member + score キーを削除するがTTLキーは削除しない ✗
return buildZSetDiffElems(keyBytes, st.origMembers, st.members)シナリオ:
補足: 非トランザクションパス（ 推奨対応: 問題2 【低】
|
1 participant
No description provided.